Switch from AddVectoredExceptionHandler to SetUnhandledExceptionFilter to avoid false positives #2334
Conversation
Codecov Report
@@ Coverage Diff @@
## devel #2334 +/- ##
==========================================
- Coverage 91.13% 90.88% -0.25%
==========================================
Files 152 151 -1
Lines 7316 7229 -87
==========================================
- Hits 6667 6570 -97
- Misses 649 659 +10 |
|
Added test to Misc.tests.cpp to validate modified SEH handling behavior on Windows. If this is the wrong test to add it to, let me know and I can move it. |
|
Can you also add a test that unhandled exception still fails tests? To hide the test from standard run, add Otherwise LGTM. |
|
I rebased the PR to contain the changes to JIT debugging, but the issue is still there: exception escaping the binary into CTest still causes wait until timeout. |
|
Let me take a look and see what is happening. |
|
Will take a look at this when I get back next year. Have a great new year. |
|
@Alan-Jowett Thanks for the PR, I am going to drop the failing test and merge the other parts. It would be appreciated if you still tried to solve the issue with the JIT debugger on AppVeyor, but even without that this is still an improvement over what is there right now. |
This avoids issues with Catch2's handler firing too early, on structured exceptions that would be handled later. This issue meant that the old attempts at structured exception handling were incompatible with Windows's ASan, because it throws continuable `C0000005` exception, which it then handles. With the new handling, Catch2 is only notified if nothing else, including the debugger, has handled the exception. Signed-off-by: Alan Jowett <alanjo@microsoft.com> Closes #2332 Closes #2286 Closes #898
|
Is it possible to backport this change to v2.x? I have hit the same issue with ASAN using LLVM/Clang. |
|
I am willing to merge a PR that does the backport (inc. tests) |
My team will try to do this, unless @alvinhochun or somebody else is already working on it. |
|
Thanks, I haven't started backporting this for real, so you can do it if you would like to. |
|
I have manually tested the fix locally and it works. I expect to submit a PR by Monday. |
Description
What
Catch2 incorrectly registers for SIGSEGV and other signals on Windows. The correct behavior is to register as the top-level unhandled exception filter so that it is notified last after the application has had an opportunity to handle the exception and after the debugger has been notified. The previous behavior was to register a vectored exception handler which results in Catch2 being notified prior to the application being able to handle the exception.
Why
Visual Studio 2022 uses structured exceptions to implement address sanitization (/fsanitize=address). This issue results in Catch2 being incompatible with a key bug-finding feature of VS 2022.
GitHub Issues
Closes #2332
Notes for maintainers
I didn't see any Windows-specific tests and I am unsure how to validate the new behavior in the CI/CD pipeline. I manually verified the change fixes the issue but would prefer to add a new test to the CI/CD pipeline if possible.
Signed-off-by: Alan Jowett alanjo@microsoft.com